Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace the version mode Mainline in 6.x (Part IV) #3883

Conversation

HHobeck
Copy link
Contributor

@HHobeck HHobeck commented Jan 25, 2024

Description

Create TrunkBasedVersionStrategy implementation and replace the version mode Mainline in 6.x.

Close #3877
Close #1839
Close #3308
Close #3570
Close #3656
Cloes #3644

Motivation and Context

For more detail please see the following ADR:

Next steps are:

  • Create TrunkBasedVersionStrategy implementation (done)
  • Create integration tests for TrunkBasedVersionStrategy (I skip this because of the limit time of reviewer)
  • Remove Mainline mode (done)
  • Replace VersioningMode with DeploymentMode (done)
  • Add new property with name strategies (type VersionStrategies[]) to the configuration root level <<-- Part of this PR
  • Create documentation for TrunkBased workflow and add breaking changes notes <<-- Part of this PR

How Has This Been Tested?

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@HHobeck HHobeck force-pushed the feature/Replace-the-version-mode-Mainline-Part-IV branch 2 times, most recently from cc5cc7c to 24b5522 Compare January 25, 2024 10:48
@arturcic
Copy link
Member

@HHobeck please move the step with unit tests at the end

@HHobeck HHobeck force-pushed the feature/Replace-the-version-mode-Mainline-Part-IV branch 2 times, most recently from 6e1ccca to dd05fea Compare January 25, 2024 12:40
BREAKING_CHANGES.md Outdated Show resolved Hide resolved
* ConfigNext
* MergeMessage
* TaggedCommit
* TrackReleaseBranches
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unsure about the name TrackReleaseBranches. We should be consistent with whether we use a verb (Track) in the enum values and whether we use singular (Commit) or plural (Branches) form.

Copy link
Contributor Author

@HHobeck HHobeck Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to any renaming as long we are renaming the underlying classes who implements the strategy as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is TrackReleaseBranches really a versioning strategy, though? Isn't it more like a branch configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment it is a version strategy yes. What is your suggestion!? To delete it? I thought this is the concept of our core domain to have provider based (strategy based) approach to determine the next version. And you can control whether or not it is called using the strategies property on the configuration root level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like the configurable strategy approach. But like next-version, tracks-release-branches is a configuration property. It's strange to have tracks-release-branches configured for a branch, but ignored because TrackReleaseBranches isn't configured.

Copy link
Contributor Author

@HHobeck HHobeck Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here I would say like I already did previously: You need to think of the property like a characteristic of a branch or a characteristic of the repository which is generally isolated of the actual used strategies. If someone decide e.g. not to use the NextVersionVersionStrategy but define next-version then it would be fine to ignore it (Actually if not it would be a bug).

And your second example is even easy to argument. The track-release-braches property will be used in the TrackReleaseBranchesVersionStrategy and TrunkBaseVersionStrategy. If you decide to use TrunkBaseVersionStrategy you should not execute the TrackReleaseBranchesVersionStrategy because it gives you maybe a wrong behavior for the TrunkBased (Mainline) workflow.

Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think of the possibility of tracks-release-branches being used by more than one strategy. That's not the case for next-version, is it?

Copy link
Contributor Author

@HHobeck HHobeck Jan 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was mistaken. I have mixed tracks-release-branches with track-merge-message in my mind. You are right tracks-release-branches will be used in TrackReleaseBranchesVersionStrategy and next-version in ConfiguredNextVersionVersionStrategy only. But that can change. Who knows!?

BREAKING_CHANGES.md Outdated Show resolved Hide resolved
docs/input/docs/reference/configuration.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the folder called SupportedWorkflows when it's associated with a configuration property called version-strategy? Either we should rename the YAML property to workflow or we should rename the folder to Strategies.

Copy link
Contributor Author

@HHobeck HHobeck Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm correct we have a clash with the term TrunkBased here... What is your suggestion? In the folder SupportedWorkflows we are talking from workflow where in the strategy we are talking from strategy which is something different.

We could define e.g. on the workflow template SupportedWorkflows/TrunkBased/v1.yml to use the strategy TrunkBased,NextVersion to support the next-version configuration as well if it is applicable. Thus there is no one to one relation from workflow to strategy. It's up to you how you would define it.

Edit:
TLDR: The folder called SupportedWorkflows is not associated with a configuration property called version-strategy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the overlap between strategy and workflow is a bit confusing. Would it be possible to remove strategy completely from configuration and only allow workflow to be configured? Perhaps not in v6, but in v7?

Copy link
Contributor Author

@HHobeck HHobeck Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I saw a lot of issues where the user wants to have it in control which strategies are used or not. The workflows are static and integrated in the assembly. If you delete the strategies then how would the user change the behavior?

Another point here is that we are not defining any hard coded business logic around the workflow. The workflow is just a template which will be used as a base configuration. If the user like he can take the configuration directly with the same effect.

Edit:
What do you think about to name the strategy instead of TrunkBasedStrategy MainlineStrategy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point wrt. allowing configurability on individual strategies. So we probably want workflow and strategies to be mutually exclusive, no? If not, how would workflow: TrunkBased with strategies: [TaggedCommit] work?

Copy link
Contributor Author

@HHobeck HHobeck Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point wrt. allowing configurability on individual strategies. So we probably want workflow and strategies to be mutually exclusive, no?

The VersionStrategies pattern is the main mechanism to control which strategy will be used to determine the next version. This can be of course changed by the user. Like every time if the user changes the configuration and customizes their own workflow he needs to ensure the correct behavior as well. Because it is hard work to build a workflow we as contributors are providing a supported workflow templates (at this moment GitHubFlow, GitFlow and TrunkBased and maybe more in future) to help the user get there build pipe up and running. And for this workflows we give a support.

Comming to your question: You only can answer this question if it is working or not if you know what workflow the user wants to implement. Because we don't know what the workflow might be we cannot do any assumptions about it. In best case it has just a performance impact in worst case the user gets a version number he would not expect.

Edit:

If not, how would workflow: TrunkBased with strategies: [TaggedCommit] work?

The scenario you are describing here would be a customization of the TrunkBased workflow. This setting would of course break everything. But the user is responsible to ensure the behavior by himself when customizing. Changing configuration is only for advanced user who understanding what they are doing. Again the used workflow template is only a base template which can be overwritten. For the version calculation GitVersion doesn't care if you use a base workspace and customize it afterwards or provide a full configuration from scratch without the usage of a base workflow. It is equivalent and the result is the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I just want us to be on the same page here and offer the best possible developer experience when interacting with these new configuration properties. That's why I thought strategies and workflow could be made mutually exclusive; you either use one or the other. GitVersion would simply return an error if both properties are present in GitVersion.yml.

I think that's going to lead to fewer problems (i.e. fewer filed issues on GitHub) than allowing developers to mix workflow and strategies. From my years of experience maintaining API surfaces like this, it's wise to start out restrictive and increase flexibility over time. If we start out flexible, allowing strategies and workflow to be mixed, it's going to be much harder adding restrictions on their usage in the future.

If we are to allow strategies and workflow simultaneously, we need to figure out whether strategies are additive or reductive to the ones implicitly added by workflow, for instance. I say we just avoid having to answer that question by not allowing it.

Copy link
Contributor Author

@HHobeck HHobeck Jan 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not a big fan of being a guardian for the user and protecting them for themselves. I would say to document it and make a attention sign that only advanced user who knows what they are doing should change this field.

Anyway if we do it like you mentioned how would the users of the following issues use this feature?

My expectation would be if you want to skip the MergeMessageVersionStrategy you need to change the configuration as following:

workflow: GitFlow/v1
strategies: [ConfiguredNextVersion, TaggedCommit, TrackReleaseBranches, VersionInBranchName]

and if you want to skip VersionInBranchNameVersionStrategy you write:

workflow: GitFlow/v1
strategies: [ConfiguredNextVersion, MergeMessage, TaggedCommit, TrackReleaseBranches]

That means the strategies array is not additive.

BREAKING_CHANGES.md Outdated Show resolved Hide resolved
* ConfigNext
* MergeMessage
* TaggedCommit
* TrackReleaseBranches
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is TrackReleaseBranches really a versioning strategy, though? Isn't it more like a branch configuration?

docs/input/docs/reference/configuration.md Outdated Show resolved Hide resolved
src/GitVersion.Configuration/ConfigurationBuilderBase.cs Outdated Show resolved Hide resolved
@asbjornu
Copy link
Member

Relevant to this PR: #1839.

@HHobeck HHobeck force-pushed the feature/Replace-the-version-mode-Mainline-Part-IV branch from 75f5dc0 to e8176a6 Compare January 26, 2024 10:31
Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are close now. The YAML array vs. flags enum thing can probably be fixed in a separate PR where we:

  1. Remove all [Json…] attributes from GitVersionConfiguration.
  2. Add a new private class inside GitVersion.Configuration.ConfigurationSerializer called GitVersionConfigurationDto that is decorated with deserialization attributes and tailored towards simple de/serialization from/to YAML.
  3. Change the implementation of GitVersion.Configuration.ConfigurationSerializer.Read() to deserialize to GitVersionConfigurationDto and then map from the DTO to the GitVersionConfiguration with manual handling of the array to Flags enum conversion.
  4. Change the implementation of GitVersion.Configuration.ConfigurationSerializer.Write() to first map from the received IGitVersionConfiguration object to GitVersionConfigurationDto and then serialize the DTO to disk.

@HHobeck
Copy link
Contributor Author

HHobeck commented Jan 26, 2024

If you not have any additional remarks please merge this PR.

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @HHobeck!

@@ -7,7 +7,7 @@
namespace GitVersion.Core.Tests.VersionCalculation.Strategies;

[TestFixture]
public class ConfigNextVersionBaseVersionStrategyTests : TestBase
public class ConfiguredNextVersionBaseVersionStrategyTests : TestBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the class name here contains BaseVersion for no particular reason?

Suggested change
public class ConfiguredNextVersionBaseVersionStrategyTests : TestBase
public class ConfiguredNextVersionStrategyTests : TestBase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@HHobeck HHobeck force-pushed the feature/Replace-the-version-mode-Mainline-Part-IV branch from 948bc9e to 0bd90f5 Compare January 28, 2024 10:45
@HHobeck
Copy link
Contributor Author

HHobeck commented Jan 28, 2024

Actually what I like to discuss with you @asbjornu: With this PR the pump messages +semver: none is not usable anymore. What are we doing with the branch related property no-bump-message?

@HHobeck HHobeck merged commit 3cafb59 into GitTools:main Jan 28, 2024
133 checks passed
Copy link
Contributor

mergify bot commented Jan 28, 2024

Thank you @HHobeck for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment